fix bug in batch SimpleFIN startDate logic #504
Conversation
actual-github-bot
bot
changed the title
matt-fidd
changed the title
|
@coderabbitai full review It seems to be struggling to pass... |
|
@matt-fidd, I'd be happy to perform a full review of the PR. I see that it's struggling to pass; I'll look into it and identify any issues. ✅ Actions performedFull review triggered. |
WalkthroughThe pull request introduces modifications to the Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/app-simplefin/app-simplefin.js (3)
215-215: Consider adding startDate validation.The comparison assumes startDate is always a valid Date object. Consider adding validation to handle edge cases.
+ if (!(startDate instanceof Date) || isNaN(startDate)) { + throw new Error('Invalid startDate provided'); + } if (transactionDate < startDate) { continue; }
213-219: Consider adding debug logging for skipped transactions.For debugging purposes, it would be helpful to log information about transactions that are being filtered out due to the date comparison.
const transactionDate = new Date(dateToUse * 1000); if (transactionDate < startDate) { + console.debug(`Skipping transaction ${trans.id}: ${transactionDate} is before ${startDate}`); continue; } newTrans.date = getDate(transactionDate);
213-219: Consider performance optimization for large transaction sets.For accounts with many transactions, the date comparison could be optimized by:
- Early filtering of transactions using the Unix timestamp directly
- Using binary search if transactions are ordered by date
Would you like me to provide a detailed implementation for these optimizations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/504.mdis excluded by!**/*.md
📒 Files selected for processing (1)
src/app-simplefin/app-simplefin.js(1 hunks)
🔇 Additional comments (2)
src/app-simplefin/app-simplefin.js (2)
213-215: LGTM! The date comparison fix looks correct.
The change properly converts the Unix timestamp to a JavaScript Date object before comparison, fixing the type mismatch issue that was causing startDate to be ignored.
219-219: LGTM! Consistent date formatting.
The date assignment uses the new transactionDate variable and maintains consistent date formatting through the getDate() utility function.
matt-fidd
force-pushed
the
simplefin-batch-fix
branch
from
5de50bc to
a7eca81
Compare
| @@ -210,12 +210,13 @@ function getAccountResponse(results, accountId, startDate) { | |||
| dateToUse = trans.posted; | |||
| } | |||
|
|
|||
| newTrans.bookingDate = getDate(new Date(dateToUse * 1000)); | |||
There was a problem hiding this comment.
Does SF not use bookingDate?
There was a problem hiding this comment.
No, all bank sync looks to use date https://github.com/actualbudget/actual/blob/c626fc2f17199a327c8dcd80e93c3549e8926240/packages/loot-core/src/server/accounts/sync.ts#L326
There was a problem hiding this comment.
The comparison here wasn't like for like and was always failing, resulting in the start date being ignored